Skip to content

fix: code review fixes for stale files#430

Open
JiayangLai wants to merge 1 commit intobirdayz:masterfrom
JiayangLai:fix/code-review-fixes
Open

fix: code review fixes for stale files#430
JiayangLai wants to merge 1 commit intobirdayz:masterfrom
JiayangLai:fix/code-review-fixes

Conversation

@JiayangLai
Copy link
Copy Markdown

Summary

Code review fixes for the 5 least recently updated files in the repository (last modified in 2020-2023):

Changes

  1. pkg/config/confluent_cloud.go

    • Fixed variable shadowing: renamed homedir variable to home to avoid shadowing the imported homedir package
    • Fixed inconsistent error message capitalization (Go error messages should start lowercase)
  2. pkg/streams/subscription_info.go

    • Added validation for negative array lengths (numPrevs and numStandby) to prevent potential integer overflow/panic when casting to int
  3. pkg/streams/decoder.go

    • Removed commented-out code that had been left in the codebase
    • Fixed getArrayLength() to properly validate negative values before the bounds check
    • Removed unused math import
  4. pkg/avro/schema.go

    • Replaced magic number 0x00 with named constant avroMagicByte for better code clarity
  5. pkg/proto/proto.go

    • Improved exclusion and deduplication logic using map[string]struct{} for O(1) lookups instead of O(n²) nested loops
    • Renamed variable p to parser to avoid potential confusion

Test Plan

  • Code builds successfully (verified with go build ./...)
  • No new lint warnings introduced

- confluent_cloud.go: Fix variable shadowing (homedir -> home) and
  inconsistent error message capitalization
- subscription_info.go: Add validation for negative array lengths
  to prevent potential panics
- decoder.go: Remove commented-out code, fix getArrayLength to
  properly validate negative values, remove unused math import
- schema.go: Replace magic number 0x00 with named constant
- proto.go: Improve exclusion and deduplication logic using maps
  for O(1) lookups instead of O(n²) nested loops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant